NEM-321 Sql query execution #112
Conversation
| def run_sql( | ||
| self, file_config: T, sql: str, params: list[Any] | None = None, read_only: bool = True | ||
| ) -> SqlExecutionResult: | ||
| return self._introspector.run_sql( |
There was a problem hiding this comment.
Nit: This method doesn't really belong in the introspector
I'm not sure it's worth it to take it out though: you'd need to refactor a bit to have a class executing SQL (including our SQL introspection queries) that is used by the Introspector
|
|
||
|
|
||
| @runtime_checkable | ||
| class SqlRunnablePlugin[T](BuildDatasourcePlugin[T], Protocol): |
There was a problem hiding this comment.
This feel like it should be its own independent protocol rather than extending BuildDatasourcePlugin
And plugins would keep on implementing BuildDatasourcePlugin but they would also implement SqlRunnablePlugin on top
I'm not fully sure how well it would work with the generics since we would want it to be the same for both the BuildDatasourcePlugin and the SqlRunnablePlugin 🤔
There was a problem hiding this comment.
Or maybe it would be easier for the run_sql to always be part of BuildDatasourcePlugin but with a default implementation throwing an exception. And instead of casting, we're always calling the method and catching the NotSupported exception
There was a problem hiding this comment.
I tried both those ways before and imo the implementation was less clear (there were issues with generics in the first case). but maybe it's worth trying somehow again
| def _apply(plugins_map): | ||
| mocker.patch( | ||
| "databao_context_engine.plugin_loader.load_plugins", | ||
| new=lambda: plugins_map, |
There was a problem hiding this comment.
I'm pretty sure the DatabaoContextPluginLoader takes an optional map of plugins (that was added specifically for tests to avoid patching)
There was a problem hiding this comment.
so in that case we'll need to patch databao_context_engine.datasources.check_config.load_plugins, right?
There was a problem hiding this comment.
or passing DatabaoContextPluginLoader instance to run_sql()
There was a problem hiding this comment.
I meant like this:
This should work to replace this patch no?
There was a problem hiding this comment.
I think you should be able to call a constructor DatabaoContextPluginLoader(your_plugins) in this test. NO need to mock anything
There was a problem hiding this comment.
probably I'm missing something, but in this test you were testing plugin loader itself, and in my case it's currently being created inside run_sql() method
| from enum import Enum | ||
|
|
||
|
|
||
| class SqlClass(Enum): |
There was a problem hiding this comment.
The naming sounds unclear. What is meant here? DatabaseAccess? DatabaseRole?
3 participants
No description provided.